[6.1] Fix 3640 | Remove extra connection deactivation.#3776
Merged
mdaigle merged 6 commits intorelease/6.1from Nov 19, 2025
Merged
[6.1] Fix 3640 | Remove extra connection deactivation.#3776mdaigle merged 6 commits intorelease/6.1from
mdaigle merged 6 commits intorelease/6.1from
Conversation
...oft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/6.1 #3776 +/- ##
===============================================
- Coverage 66.69% 65.86% -0.83%
===============================================
Files 279 279
Lines 61764 61765 +1
===============================================
- Hits 41192 40681 -511
- Misses 20572 21084 +512
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cheenamalhotra
approved these changes
Nov 19, 2025
paulmedynski
approved these changes
Nov 19, 2025
apoorvdeshmukh
added
Area\Engineering
This was referenced Jan 15, 2026
This was referenced Feb 9, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
This was referenced Feb 16, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When I originally implemented #3019, due to a misunderstanding about when connections are activated/deactivated, I added an extraneous call to DbConnectionInternal.DeactivateConnection as part of the PutObjectFromTransactedPool. Connections are always deactivated at the time they are returned to the pool via the ReturnInternalConnection method and do not need to be activated/deactivated as they move between the main pool and the transacted pool. The activate and deactivate methods modify a counter value that tracks the number of connections in active use. This extra deactivate led to active connection counts going negative.
However, removing this DeactivateConnection call is also not entirely correct. Part of the deactivation process is setting the "reset" flags via a call to the ResetConnection method. These flags determine the Status value sent to the server when a connection is recycled out of the pool. Connections that are placed in the transacted pool may use status RESETCONNECTIONSKIPTRAN if they are participating in a distributed transaction. This gives extra guarantees that the server will not reset the connection's transaction state. When moving a connection out of the transacted pool and back into the main pool, we no longer want the server to maintain the association to the transaction because the transaction has ended and we want to use the connection for general purpose commands. Therefore, we need to downgrade the connection's status value to RESETCONNECTION. This way, the next time the connection is used its association with the transaction will be removed.
To appropriately set the reset flags without impacting the active connection counter, I exposed the ResetConnection method as internal so that it can be called directly by the pool. All other actions taken in DeactivateConnection are safe to skip at this point.
I hope to clean up this flow as part of adding transaction support to the new pool implementation, but it's out of scope for this PR.
Issues
#3640
Testing
None of our tests cover metrics. Working on adding that as part of the new connection pool work.
In the meantime, this change will be verified manually.
Existing distributed transaction tests show that the reset flags are still reevaluated correctly.